Skip to content

Conversation

@mslwang
Copy link
Collaborator

@mslwang mslwang commented Dec 1, 2024

Notion ticket link

Add startup Logs (Fix logging) for backend

Implementation description

  • We were having issues troubleshooting where some people may have startup issues, since errors weren't being emitted to the console (through standard error). After deep diving, we realized that the alembic.ini config was taking control over logging configurations and suppressing other loggers. Given that going without the alembic.ini config doesn't seem viable at the moment, I've stuck to basing all loggers in the API off of one sublogger, and using a function to create child loggers for more context.

Steps to test

  1. Run pdm run dev
  2. Add a new logger using the instructions in the README to another service, ie.
    To add a logger to a new service or file, use the LOGGER_NAME function in app/utilities/constants.py
from app.utilities.constants import LOGGER_NAME

log = logging.getLogger(LOGGER_NAME("my_service"))

What should reviewers focus on?

  • Whether logging works

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@mslwang mslwang requested review from Mayank808 and mmiqball December 1, 2024 04:36
@ebwu95 ebwu95 force-pushed the mslwang/LLSC-51-add-startup-logs branch from 1348940 to 6f0bfa4 Compare February 11, 2025 02:35
precommit = "pre-commit run"
precommit-install = "pre-commit install"
dc-down = "docker-compose down -v"
dc-up = "docker-compose up -d"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, Matt mentioned that we don't really need docker containers other than the database container. So even if you just do pdm run dev + docker compose up -d db for only the database container, that is enough. Maybe something to look into in another PR? If we don't need them we can just remove the other containers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good idea, I'll look into that for another PR

@ebwu95 ebwu95 merged commit 6c06f16 into main Feb 12, 2025
1 check passed
@ebwu95 ebwu95 deleted the mslwang/LLSC-51-add-startup-logs branch February 12, 2025 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants